Improve find and package support #264

niosHD · 2016-01-12T12:11:53Z

This PR upgrades cmake to 2.8.12 to implement proper cmake find_package support using config and export file generation. Having this support enables users to use installed cppformat with a simple find_package call. Directly using a version from a build directory is also supported. I think that this should also resolve #257.

I am aware that upgrading cmake is maybe not desired. However, 2.8.12 is already pretty old and even available on the current (and soon superseded) Ubuntu 14.04 LTS. It should further be no problem to build a current CMake from source, especially for a software developer which wants to use cppformat.

Additionally, packaging in out of source build directories is now supported.

Regards,
niosHD

Usage Example:

main.cpp:

#include <cppformat/format.h>
int main(int argc, char** argv)
{
  for(int i = 0; i < argc; ++i)
    fmt::print("{}: {}\n",i,argv[i]);
  return 0;
}

CMakeLists.txt:

cmake_minimum_required(VERSION 2.8.12)

project(cppformat-test)

find_package(cppformat REQUIRED)

add_executable(cppformat-test "main.cpp")
target_link_libraries(cppformat-test cppformat)

Configuring when cppformat is installed under CMAKE_INSTALL_PREFIX: cmake <PATH_TO_TEST_SRC>

Configuring when cppformat is installed ELSEWHERE: cmake -Dcppformat_DIR=<ELSEWHERE>/lib/cmake/cppformat <PATH_TO_TEST_SRC>

Configuring when cppformat is only built: cmake -Dcppformat_DIR=<cppformat_BUILD_DIR> <PATH_TO_TEST_SRC>

This commit upgrades cmake to 2.8.12 to implement proper cmake `find_package` support using config and export file generation. Having this support enables users to use installed cppformat with a simple `find_package` call. Directly using a version from a build directory is also supported. main.cpp: ``` #include <cppformat/format.h> int main(int argc, char** argv) { for(int i = 0; i < argc; ++i) fmt::print("{}: {}\n",i,argv[i]); return 0; } ``` CMakeLists.txt: ``` cmake_minimum_required(VERSION 2.8.12) project(cppformat-test) find_package(cppformat REQUIRED) add_executable(cppformat-test "main.cpp") target_link_libraries(cppformat-test cppformat) ``` Configuring when cppformat is installed under `CMAKE_INSTALL_PREFIX`: `cmake <PATH_TO_TEST_SRC>` Configuring when cppformat is installed `ELSEWHERE`: `cmake -Dcppformat_DIR=<ELSEWHERE>/lib/cmake/cppformat <PATH_TO_TEST_SRC>` Configuring when cppformat is only built: `cmake -Dcppformat_DIR=<cppformat_BUILD_DIR> <PATH_TO_TEST_SRC>`

vitaut · 2016-01-12T14:56:58Z

CMakeLists.txt

@@ -156,15 +156,52 @@ if (EXISTS .gitignore)

  set(CPACK_SOURCE_GENERATOR ZIP)
  set(CPACK_SOURCE_IGNORE_FILES ${ignored_files})
-  set(CPACK_SOURCE_PACKAGE_FILE_NAME cppformat-${CPPFORMAT_VERSION})


Why remove CPACK_SOURCE_PACKAGE_FILE_NAME? It is used to set the correct source package name for distribution via website and releases.

I replaced it with CPACK_PACKAGE_NAME which is used for all generators, not only the source ZIP generator. Binary packages generated via make package are now also named correctly.

However, it seems like I have missed that it now generates cppformat-<version>-Source.zip instead of cppformat-<version>.zip which definitely was not intended. Good catch!

vitaut · 2016-01-12T15:08:06Z

Awesome, thanks for the contribution! I'm fine with increasing CMake version requirement to 2.8.12, but a newer version of CMake should be installed on Travis then, because the current version is 2.8.7 causing build failure: https://travis-ci.org/cppformat/cppformat/jobs/101819674

Also a minor comment regarding CPACK_SOURCE_PACKAGE_FILE_NAME above.

niosHD · 2016-01-12T15:16:02Z

Great, you are welcome! I will address your comment and also have a look at the travis config to fix the CI issue.

The use of `CPACK_PACKAGE_NAME` leads to `cppformat-<version>-Source.zip` as the name for the source package which is different from the expected `cppformat-<version>.zip`.

Whitelist of repos: https://github.com/travis-ci/apt-source-whitelist/blob/master/ubuntu.json Whitelist of packages: https://github.com/travis-ci/apt-package-whitelist/blob/master/ubuntu-precise

Improve find and package support

vitaut · 2016-01-13T15:16:15Z

Merged with some minor, but opinionated =) changes in 22f6114 mostly to improve consistency with existing code. Thanks!

niosHD · 2016-01-13T15:33:45Z

Haha, consistency is definitely desired. =)

Anyway, thank you for this great library and the fast and responsive processing of this pull request!

trygvis · 2016-01-13T20:57:03Z

Is it possible to use this in combination with ExternalProject_Add and/or git subtree?

vitaut · 2016-01-13T22:36:56Z

Not sure about ExternalProject_Add but if you have cppformat's code in your repo via subtree or other method, then you can simply use add_subdirectory(path/to/cppformat) in CMake.

niosHD · 2016-01-13T23:58:55Z

I have not tested it right now but it should work with ExternalProject_Add without a problem. It even simplifies this use case given that projects which use the library do not need to know what exact cppformat version it is, if it was build as static or dynamic library, or how the library file is exactly named.

However, I want to stress that this is only true for cmake powered super builds which exclusively build external projects.

If you want to use ExternalProject_Add in a project for dependencies of add_library or add_executable then this PR does not help you at all. This is due to a limitation/design decision of CMake. CMake needs the config files for find_package, which are now build at generation time of the library build system, at configuration/generation time of the project build system. ExternalProject on the other hand is only executed at build time which obviously is later in time. Use the add_subdirectory technique, which already has been mentioned by vitaut, instead if this is your use case.

Hope this helps,
niosHD

niosHD · 2016-01-26T09:47:15Z

@vitaut I just realized that your changes in 22f6114 changed the behavior when the library is used from the build tree.

I previously copied the header into the build tree under cppformat/format.h and exported the build directory as include directory. The new version omits the copy and directly exports the current source directory. I definitely would preferred this solution as well, but they are unfortunately not equivalent.

The problem of this solution is that the required include paths are different. Including an installed version is done via #include <cppformat/format.h>. Including cppformat using the exported target from the build directory on the other hand now is done via #include <format.h>.

We definitely should fix this discrepancy given that an application should not be aware of the way cppformat is built/installed.

I see three possible solutions:

Move the source files itself into a cppformat directory. However, this most likely breaks current users which simply include the cppformat source directory.
Reintroduce the copy to the build directory. This is more like a workaround but has the advantage that nothing changes for current users.
Remove the export feature from the build tree completely.

Suggestions?

Regards,
Mario

vitaut · 2016-01-26T15:35:30Z

I'd go with one of the following options:

Require users to maintain the build directory naming consistent with whatever includes they use, so if they want to include cppformat/format.h, they should build in the directory named cppformat (in particular, this will work with an in-source build with the default repo name). Or they can simply include format.h and configure include directories accordingly.
Make export from build directory a configuration option disabled by default and copy files only if it is enabled. Kind of like your second option but controlled by a CMake flag.

What do you think?

niosHD · 2016-01-26T21:08:20Z

I don't really like to force the user to mess with the include directories later on. The whole point of the export and find scripts is to get this right. I therefore would go with your second option.

What do you think if we make the export, or at least the header copy, conditional but instead of introducing a new option we only do it for out-of-source builds? In-source builds would than behave like right now and out-of source builds would behave like in the original PR.

vitaut · 2016-01-27T16:21:41Z

I don't really like to force the user to mess with the include directories later on. The whole point of the export and find scripts is to get this right.

Fair enough.

What do you think if we make the export, or at least the header copy, conditional but instead of introducing a new option we only do it for out-of-source builds?

The point is to avoid copying headers for out-of-source builds, for in-source CMake might be clever enough not to do this by itself.

Thinking more of it, I'm starting to lean towards your first option, namely moving the source files to a cppformat subdirectory. This will break some current users who do add_subdirectory, but we'll have to do it anyway at some point as the library grows. It can be combined with some improvements to avoid breakage in the future, e.g. adding an interface include directory for the cppformat target so that users could just do

target_link_library(<target> cppformat)

and it would set up include directories without the need to specify them manually.

niosHD · 2016-01-27T17:03:37Z

On 01/27/2016 05:22 PM, Victor Zverovich wrote:

What do you think if we make the export, or at least the header copy, conditional but instead of introducing a new option we only do it for out-of-source builds?

The point is to avoid copying headers for out-of-source builds, for in-source CMake might be clever enough not to do this by itself.

Understood. I previously assumed that you just want to get rid of this
behavior for in-source builds because it messes with the working copy.
Thank you for clarifying.

Thinking more of it, I'm starting to lean towards your first option, namely moving the source files to a cppformat subdirectory. This will break some current users who do add_subdirectory, but we'll have to do it anyway at some point as the library grows. It can be combined with some improvements to avoid breakage in the future, e.g. adding an interface include directory for the cppformat target so that users could just do

I am certain that this would be the cleanest solution. I was just
reserved about it given that it is quite invasive.

target_link_library(<target> cppformat)
and it would set up include directories without the need to specify them manually.

Right, this is exactly the nice behavior which you get from the new find
scripts. If we get the same nice usability when we include cppformat via
add_subdirectory that would be awesome.

I will give it a try tomorrow and submit a PR as soon as I have
something satisfying.

vitaut · 2016-01-27T17:14:08Z

Great! BTW I think breakage can be avoided in most cases by having the following format.h in the top level:

#include "cppformat/format.h"
#warning "Including format.h from the top-level directory is deprecated."

or something like that.

niosHD added 2 commits January 12, 2016 12:47

enable package support for out of source builds

b732455

vitaut reviewed Jan 12, 2016
View reviewed changes

niosHD added 3 commits January 12, 2016 16:48

reverted removal of CPACK_SOURCE_PACKAGE_FILE_NAME

3fc3ecd

The use of `CPACK_PACKAGE_NAME` leads to `cppformat-<version>-Source.zip` as the name for the source package which is different from the expected `cppformat-<version>.zip`.

removed workaround for cmake versions prior to 2.8.10

ef7bbff

vitaut added a commit that referenced this pull request Jan 13, 2016

Merge pull request #264 from niosHD/improve-find-and-package-support

52ee516

Improve find and package support

vitaut merged commit 52ee516 into fmtlib:master Jan 13, 2016

vitaut mentioned this pull request Jan 13, 2016

CMakeLists file missing export #257

Closed

niosHD mentioned this pull request Jan 31, 2016

Update project layout #267

Merged

niosHD deleted the improve-find-and-package-support branch February 4, 2016 13:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve find and package support #264

Improve find and package support #264

niosHD commented Jan 12, 2016

vitaut Jan 12, 2016

niosHD Jan 12, 2016

vitaut commented Jan 12, 2016

niosHD commented Jan 12, 2016

vitaut commented Jan 13, 2016

niosHD commented Jan 13, 2016

trygvis commented Jan 13, 2016

vitaut commented Jan 13, 2016

niosHD commented Jan 13, 2016

niosHD commented Jan 26, 2016

vitaut commented Jan 26, 2016

niosHD commented Jan 26, 2016

vitaut commented Jan 27, 2016

niosHD commented Jan 27, 2016

vitaut commented Jan 27, 2016

Improve find and package support #264

Improve find and package support #264

Conversation

niosHD commented Jan 12, 2016

Usage Example:

vitaut Jan 12, 2016

Choose a reason for hiding this comment

niosHD Jan 12, 2016

Choose a reason for hiding this comment

vitaut commented Jan 12, 2016

niosHD commented Jan 12, 2016

vitaut commented Jan 13, 2016

niosHD commented Jan 13, 2016

trygvis commented Jan 13, 2016

vitaut commented Jan 13, 2016

niosHD commented Jan 13, 2016

niosHD commented Jan 26, 2016

vitaut commented Jan 26, 2016

niosHD commented Jan 26, 2016

vitaut commented Jan 27, 2016

niosHD commented Jan 27, 2016

vitaut commented Jan 27, 2016